refactor(transport): single-owner HTTP transport, fix util layering inversion#1213
Conversation
|
liangshuo-1 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThis PR consolidates HTTP transport and proxy-handling utilities by migrating proxy-aware transport, warning, and client helpers into a new ChangesHTTP Transport Consolidation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1213 +/- ##
=======================================
Coverage 69.21% 69.22%
=======================================
Files 633 634 +1
Lines 59563 59555 -8
=======================================
- Hits 41227 41224 -3
+ Misses 15015 15012 -3
+ Partials 3321 3319 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6a540bab4179e68464c08f7e16672aa14a67f401🧩 Skill updatenpx skills add larksuite/cli#refactor/transport-single-owner -y -g |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/transport/config.go (1)
4-11: 💤 Low valueStale package doc comment.
The doc comment on line 4 still references
proxypluginbut the package is nowtransport. Update for consistency.📝 Proposed fix
-// Package proxyplugin implements the ~/.lark-cli/proxy_config.json based security proxy plugin mode. +// Package transport implements the ~/.lark-cli/proxy_config.json based security proxy plugin mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/transport/config.go` around lines 4 - 11, Update the stale package doc comment that still says "proxyplugin" so it matches the current package name "transport": edit the top-of-file package comment (the block above the package transport declaration) to replace "proxyplugin" with "transport" and ensure any references in that comment (e.g., descriptions of behavior or filenames) remain accurate and consistent with the transport package's responsibilities; keep the rest of the comment content intact unless additional wording is needed for correctness.internal/cmdutil/transport.go (1)
176-186: 💤 Low valueParameter
transportnow shadows the importedtransportpackage.With
internal/transportnewly imported (Line 12), thetransport http.RoundTripperparameter shadows the package name insidewrapWithExtension. It compiles since the package isn't referenced here, but it's a readability/govet -shadowsnag introduced by this PR. Consider renaming the parameter.♻️ Proposed rename
-func wrapWithExtension(transport http.RoundTripper) http.RoundTripper { +func wrapWithExtension(rt http.RoundTripper) http.RoundTripper { p := exttransport.GetProvider() if p == nil { - return transport + return rt } tr := p.ResolveInterceptor(context.Background()) if tr == nil { - return transport + return rt } - return &extensionMiddleware{Base: transport, Ext: tr, ExtName: p.Name()} + return &extensionMiddleware{Base: rt, Ext: tr, ExtName: p.Name()} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmdutil/transport.go` around lines 176 - 186, The parameter name transport in wrapWithExtension shadows the imported transport package; rename the parameter (e.g., baseTransport or rt) in the function signature and update its usage inside wrapWithExtension and the returned extensionMiddleware construction (referencing extensionMiddleware{Base: ..., Ext: tr, ExtName: p.Name()}) so the package name is no longer shadowed and govet -shadow warnings are resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmdutil/factory_default.go`:
- Line 105: WarnIfProxied writes plain-text to the provided writer which can
emit warnings to f.IOStreams.ErrOut during credential resolution and break the
stderr JSON envelope; modify cachedHttpClientFunc and cachedLarkClientFunc to
avoid calling transport.WarnIfProxied with f.IOStreams.ErrOut during token
resolution path (DefaultTokenProvider.resolveUAT → p.httpClient()), instead gate
the warning emission behind the JSON envelope state or send warnings to a
non-JSON sink (e.g., an internal logger) so that WarnIfProxied is not invoked
with f.IOStreams.ErrOut during credential resolution; locate usages of
transport.WarnIfProxied, cachedHttpClientFunc, cachedLarkClientFunc and
DefaultTokenProvider.resolveUAT and change the callsite to either skip
WarnIfProxied or route its output away from f.IOStreams.ErrOut when credential
resolution is in progress.
In `@internal/transport/transport_test.go`:
- Around line 151-156: Update the stale doc comment header and references to
match the renamed test and current package: change the header name from
TestSharedTransport_InvalidConfigBlockerIsConcreteTransport to
TestPluginTransport_InvalidConfigBlockerIsConcreteTransport and replace the two
occurrences of util.Fallback with transport.Fallback so the comment accurately
documents the fail-closed invariant guarded by
TestPluginTransport_InvalidConfigBlockerIsConcreteTransport in this package.
---
Nitpick comments:
In `@internal/cmdutil/transport.go`:
- Around line 176-186: The parameter name transport in wrapWithExtension shadows
the imported transport package; rename the parameter (e.g., baseTransport or rt)
in the function signature and update its usage inside wrapWithExtension and the
returned extensionMiddleware construction (referencing extensionMiddleware{Base:
..., Ext: tr, ExtName: p.Name()}) so the package name is no longer shadowed and
govet -shadow warnings are resolved.
In `@internal/transport/config.go`:
- Around line 4-11: Update the stale package doc comment that still says
"proxyplugin" so it matches the current package name "transport": edit the
top-of-file package comment (the block above the package transport declaration)
to replace "proxyplugin" with "transport" and ensure any references in that
comment (e.g., descriptions of behavior or filenames) remain accurate and
consistent with the transport package's responsibilities; keep the rest of the
comment content intact unless additional wording is needed for correctness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c492fb9a-8a79-4ee1-b092-03d268a3c2c4
📒 Files selected for processing (19)
cmd/config/init_interactive.gocmd/doctor/doctor.gointernal/auth/transport.gointernal/cmdutil/factory_default.gointernal/cmdutil/transport.gointernal/cmdutil/transport_test.gointernal/registry/remote.gointernal/transport/config.gointernal/transport/config_test.gointernal/transport/shared.gointernal/transport/shared_test.gointernal/transport/tls_ca.gointernal/transport/tls_ca_test.gointernal/transport/transport.gointernal/transport/transport_test.gointernal/transport/warn.gointernal/transport/warn_test.gointernal/update/update.gointernal/util/proxy.go
💤 Files with no reviewable changes (1)
- internal/util/proxy.go
…x util layering inversion internal/util imported internal/proxyplugin (SharedTransport, FallbackTransport, NewHTTPClient, and WarnIfProxied via proxyPluginStatus), so a foundational util package depended up into a feature package, pulling binding/core/vfs into the transitive cone of every util importer. Move internal/proxyplugin -> internal/transport and make it the single owner of outbound transport: fold the two SharedTransport functions into one Shared() (proxy-plugin override -> LARK_CLI_NO_PROXY -> http.DefaultTransport), and move Fallback/NewHTTPClient/WarnIfProxied/DetectProxyEnv/noProxyTransport out of the now-deleted internal/util/proxy.go into the new package. The proxy-plugin probe is demoted to a private pluginTransport(); the duplicate redactProxyURL collapses to one. internal/util keeps no proxy code and is a leaf again. Re-point all consumers (registry, doctor, config, auth, cmdutil, update) to internal/transport. Behavior-preserving: package move + symbol rename + dedup. Two new tests lock the fail-closed contract (plugin overrides NO_PROXY; malformed config never falls through to direct egress). Change-Id: I3e5b596e1a7f70e55995897140738a74ad4d6fd1
f6be704 to
6a540ba
Compare
Resolve internal/registry/remote.go import conflict by keeping BOTH sides: the typed meta model (this branch) and the proxy-aware transport client (main #1181/#1213). The changes are orthogonal — fetchRemoteMerged now uses transport.NewHTTPClient(fetchTimeout) over the typed MergedRegistry. Only the import block conflicted; the transport client line auto-merged. Change-Id: Ia61ab32c39e5b4eadb05e7efe46ff0c8dcfce568
Resolve internal/registry/remote.go import conflict by keeping BOTH sides: the typed meta model (this branch) and the proxy-aware transport client (main #1181/#1213). The changes are orthogonal — fetchRemoteMerged now uses transport.NewHTTPClient(fetchTimeout) over the typed MergedRegistry. Only the import block conflicted; the transport client line auto-merged.
…x util layering inversion (larksuite#1213) internal/util imported internal/proxyplugin (SharedTransport, FallbackTransport, NewHTTPClient, and WarnIfProxied via proxyPluginStatus), so a foundational util package depended up into a feature package, pulling binding/core/vfs into the transitive cone of every util importer. Move internal/proxyplugin -> internal/transport and make it the single owner of outbound transport: fold the two SharedTransport functions into one Shared() (proxy-plugin override -> LARK_CLI_NO_PROXY -> http.DefaultTransport), and move Fallback/NewHTTPClient/WarnIfProxied/DetectProxyEnv/noProxyTransport out of the now-deleted internal/util/proxy.go into the new package. The proxy-plugin probe is demoted to a private pluginTransport(); the duplicate redactProxyURL collapses to one. internal/util keeps no proxy code and is a leaf again. Re-point all consumers (registry, doctor, config, auth, cmdutil, update) to internal/transport. Behavior-preserving: package move + symbol rename + dedup. Two new tests lock the fail-closed contract (plugin overrides NO_PROXY; malformed config never falls through to direct egress).
Summary
Make
internal/transport(renamed frominternal/proxyplugin) the single owner of outbound HTTP transport, and remove the layering inversion that appeared when proxy-plugin mode was wired intointernal/util.Motivation
After #1181,
internal/utilimportedinternal/proxypluginin four places (SharedTransport,FallbackTransport,NewHTTPClient, andWarnIfProxiedviaproxyPluginStatus). A foundational utility package — imported by ~18 packages — depended up into a feature package, pullingbinding/core/vfsinto the transitive cone of everyutilimporter. The transport logic was also duplicated: twoSharedTransportand tworedactProxyURLimplementations split across the two packages.Changes
internal/proxyplugin→internal/transport; it now owns all transport assembly.SharedTransportinto onetransport.Shared()(precedence: proxy-plugin override →LARK_CLI_NO_PROXY→http.DefaultTransport); demote the proxy-plugin probe to a privatepluginTransport().Fallback/NewHTTPClient/WarnIfProxied/DetectProxyEnv/noProxyTransportout ofinternal/util/proxy.go(deleted) into the new package; collapse the duplicateredactProxyURLto one.internal/utilkeeps no proxy code, so it is a leaf again — theutil → proxypluginimport edge is gone.registry,doctor,config,auth,cmdutil,update) tointernal/transport.Behavior-preserving: package move + symbol rename + de-duplication. No functional change to proxy-plugin mode, the
NewHTTPClientegress chokepoint, the fail-closed transport, or the proxy/MITM warnings introduced in #1181.Test Plan
go build ./...go vet(affected packages)gofmt -lcleango testgreen:transport,util,cmdutil,auth,update,registry,doctor,configLARK_CLI_NO_PROXY; a malformedproxy_config.jsonnever falls through to direct egress.Summary by CodeRabbit